Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fabo/1199 improve jank at start #1216

Merged
merged 48 commits into from
Sep 4, 2018
Merged

Conversation

faboweb
Copy link
Collaborator

@faboweb faboweb commented Aug 30, 2018

Closes #1199, #1211, #1080

Also fixes a bug on the PageBond where the atoms would not have been updated after staking (optimistic update).

❤️ Thank you!

@faboweb faboweb requested review from NodeGuy and nylira as code owners August 30, 2018 10:57
@codecov
Copy link

codecov bot commented Sep 3, 2018

Codecov Report

Merging #1216 into develop will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #1216      +/-   ##
===========================================
+ Coverage    95.89%   95.91%   +0.01%     
===========================================
  Files           81       82       +1     
  Lines         1608     1615       +7     
  Branches        75       76       +1     
===========================================
+ Hits          1542     1549       +7     
  Misses          60       60              
  Partials         6        6
Impacted Files Coverage Δ
app/src/renderer/components/staking/PageBond.vue 100% <ø> (ø) ⬆️
app/src/renderer/components/staking/LiDelegate.vue 91.66% <ø> (ø) ⬆️
.../src/renderer/components/staking/PageValidator.vue 100% <100%> (ø) ⬆️
app/src/renderer/vuex/modules/keybase.js 100% <100%> (ø)
...pp/src/renderer/components/staking/PageStaking.vue 100% <100%> (ø) ⬆️
app/src/renderer/vuex/modules/wallet.js 95.08% <100%> (-0.51%) ⬇️
app/src/renderer/vuex/modules/user.js 100% <100%> (ø) ⬆️
app/src/renderer/vuex/getters.js 100% <100%> (ø) ⬆️
app/src/renderer/vuex/modules/delegates.js 100% <100%> (ø) ⬆️
app/src/renderer/vuex/modules/node.js 96.77% <100%> (ø) ⬆️
... and 3 more

@faboweb faboweb changed the title WIP: Fabo/1199 improve jank at start Fabo/1199 improve jank at start Sep 3, 2018
@@ -100,7 +100,7 @@ export default ({ node }) => {
transactions.map(async t => {
let blockMetaInfo = await dispatch("queryBlockInfo", t.height)
t.time = blockMetaInfo && blockMetaInfo.header.time
return transactions
return t
Copy link
Collaborator

@jbibla jbibla Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would vote for transactions over t - i'm sure @NodeGuy feels the same way

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transactions was a bug, since the variable didn't exist in the scope of the function...

Copy link
Collaborator Author

@faboweb faboweb Sep 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding: I would like to avoid refactoring existing untouched code in PRs so we do not bloat the PRs. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah i see - ok makes sense. thank you for clarifying.

@@ -38,8 +38,8 @@ tm-page.page-bond(title="Staking")
:class="bondGroupClass(delta(d.atoms, d.oldAtoms))")
.bond-group__fields
.bond-bar
label.bond-bar__label(v-if="!d.delegate.revoked") {{ d.delegate.moniker }}
label.bond-bar__label.revoked(v-if="d.delegate.revoked") {{ d.delegate.moniker }}
label.bond-bar__label(v-if="!d.delegate.revoked") {{ d.delegate.description.moniker }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed monikers not showing

@@ -44,7 +44,11 @@ export default function({ node }) {
rootState.wallet.zoneIds.unshift(header.chain_id)
}

await dispatch("maybeUpdateValidators", header)
// updating the header is done even while the user is not logged in
Copy link
Collaborator

@jbibla jbibla Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we do this / is this still true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we bind to the "new header" events before signing in. the event would then trigger an update of the validatorset which is skipped here. This will also change when we react to the validatorset updated event.

},
candidates[2] // this is the revoked one
)
let chileanValidator = Object.assign(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🇨🇱

moniker: "someOtherValidator"
description: {
description: "descriptionY",
country: "Canada",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🇨🇦

Copy link
Collaborator

@jbibla jbibla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great to me.

not sure i understand why / how separating keybase out from the validator getter is a good thing but i believe you.

also not sure i understand why the atomDiff needs to be calculated the way it does.

approved nonetheless.

side note: i would LOVE if we refactored just the language because delegates and delegators feels very old and unfamiliar. what is committedDelegates?

@faboweb
Copy link
Collaborator Author

faboweb commented Sep 4, 2018

side note: i would LOVE if we refactored just the language because delegates and delegators feels very old and unfamiliar. what is committedDelegates?

Love the idea. Can you create an issue for that?

not sure i understand why / how separating keybase out from the validator getter is a good thing but i believe you.

keybase is a completely different ressource then where we get our validators from. so I thought we should split these concerns.

also not sure i understand why the atomDiff needs to be calculated the way it does.

how would you do it? maybe my approach was too much

@jbibla
Copy link
Collaborator

jbibla commented Sep 4, 2018

done #1283

@fedekunze fedekunze merged commit 9fe51d8 into develop Sep 4, 2018
@fedekunze fedekunze deleted the fabo/1199-improve-jank-at-start branch September 4, 2018 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jank while loading validator profiles
3 participants